-
Notifications
You must be signed in to change notification settings - Fork 0
Autogenerate sockets through adj #54
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
This reverts commit 53a4131.
…al packet sending
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request introduces a mechanism to autogenerate socket and packet transmission configuration through the ADJ (Adjacency) system. The changes add socket definitions and packet timing information to enable code generation for network communication.
Changes:
- Added new
sockets.jsonfile defining network socket configurations (ServerSocket, DatagramSocket, and Socket types) for VCU board - Enhanced packet definitions with transmission period (
period,period_type) and target socket (socket) fields - Updated board configuration to reference the new sockets definition file
- Extended README documentation with socket schema, field descriptions, and usage examples
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 17 comments.
| File | Description |
|---|---|
| boards/VCU/sockets.json | New file defining 6 network sockets (3 TCP, 3 UDP) for control station, PCU, and HVSCU communication |
| boards/VCU/packets.json | Added period timing and socket routing information to 8 data packets |
| boards/VCU/VCU.json | Integrated sockets configuration file reference into board configuration |
| README.md | Added comprehensive documentation for sockets schema, updated packet schema with new fields, and added configuration rules |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| 5. **ID Uniqueness**: Board IDs must be unique across all boards. Packet IDs should be unique within their type category. | ||
|
|
||
| 6. **Socket Naming**: The sockets referenced inside the packet definitions must use exact names as defined in `socket.json`. |
Copilot
AI
Jan 23, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reference should be "sockets.json" (plural) to match the actual filename, not "socket.json".
| 6. **Socket Naming**: The sockets referenced inside the packet definitions must use exact names as defined in `socket.json`. | |
| 6. **Socket Naming**: The sockets referenced inside the packet definitions must use exact names as defined in `sockets.json`. |
README.md
Outdated
| "name": "Brake Telemetry", | ||
| "variables": ["brake_pressure", "brake_status", "brake_temperature"] | ||
| "variables": ["brake_pressure", "brake_status", "brake_temperature"], | ||
| "period_ms": 16.67, |
Copilot
AI
Jan 23, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The example uses "period_ms" as a field name, but the actual implementation uses two separate fields: "period_type" and "period". The example should be updated to match the actual schema being used throughout packets.json.
| "period_ms": 16.67, | |
| "period_type": "milliseconds", | |
| "period": 16.67, |
| { | ||
| "type": "DatagramSocket", | ||
| "name": "hvscu_udp", | ||
| "remote_ip":"192.168.1.7", |
Copilot
AI
Jan 23, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistent spacing after colons in JSON. Line 46 has "remote_ip":"192.168.1.7" without a space after the colon, while other fields have spaces after colons. For consistency, consider adding a space after the colon.
| ```json | ||
| { | ||
| "type": "string", | ||
| "name": "string", | ||
| "port": "number?", | ||
| "local_port":"number?", | ||
| "remote_ip": "string?", | ||
| "remote_port":"number?" | ||
|
|
||
| } |
Copilot
AI
Jan 23, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The schema shows a single socket object, but the actual sockets.json file contains an array of socket objects. The schema should be wrapped in array brackets to accurately represent the structure. Change the opening brace on line 221 to an opening bracket and update line 229 accordingly.
| "period_type": "us", | ||
| "period": 16.67, |
Copilot
AI
Jan 23, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The period is set to 16.67 with period_type "us" (microseconds). This seems inconsistent with the other packets that use "ms" (milliseconds) with the same period value of 16.67. A period of 16.67 microseconds would be extremely fast (approximately 60,000 Hz). This is likely intended to be "ms" for a ~60 Hz update rate, consistent with the other packets.
| ``` | ||
|
|
||
| **Field Descriptions:** | ||
| - `type`: Socket type string(e.g., "ServerSocket","DatagramSocket","Socket") |
Copilot
AI
Jan 23, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing space after colon in the field description. Should be "string (e.g.," with a space after the colon instead of "string(e.g.,".
| - `type`: Socket type string(e.g., "ServerSocket","DatagramSocket","Socket") | |
| - `type`: Socket type string (e.g., "ServerSocket","DatagramSocket","Socket") |
| - `period_type`: Optional string specifying the type of measurement for period, either microseconds | ||
| or milliseconds | ||
| - `period`: Optional number specifying the transmission period in milliseconds or microseconds for periodic packets. Can be an integer or floating-point value | ||
| - `socket`: Optinal string that should match the name of the socket you want to transmit this packet trhough |
Copilot
AI
Jan 23, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The word "Optinal" should be spelled "Optional".
| - `socket`: Optinal string that should match the name of the socket you want to transmit this packet trhough | |
| - `socket`: Optional string that should match the name of the socket you want to transmit this packet trhough |
| { | ||
| "type": "DatagramSocket", | ||
| "name": "control_station_udp", | ||
| "remote_ip":"192.168.0.9", |
Copilot
AI
Jan 23, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistent spacing after colons in the example JSON. Line 252 has "remote_ip":"192.168.0.9" without a space after the colon, while other fields in the example have spaces. For consistency, add a space after the colon.
| "remote_ip":"192.168.0.9", | |
| "remote_ip": "192.168.0.9", |
|
|
||
|
|
||
|
|
||
|
|
||
|
|
Copilot
AI
Jan 23, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excessive blank lines at the end of the file. Lines 50-54 contain only whitespace and should be removed for cleaner formatting.
|
|
||
|
|
Copilot
AI
Jan 23, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Blank lines 39-40 appear to contain only whitespace. Consider removing these extra blank lines for cleaner formatting.
|
ADJ-Validator, now suports this PR. |
Now all sockets and packet period time will be described through adj, allowing the possibility of autogenerating all the code